-
Notifications
You must be signed in to change notification settings - Fork 12
Move to ES bridge API #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/main/java/co/elastic/logstash/filters/elasticintegration/EventProcessor.java
Show resolved
Hide resolved
extensions.add(new ConstantKeywordPainlessExtension()); // module: constant-keyword | ||
extensions.add(new ProcessorsWhitelistExtension()); // module: ingest-common | ||
extensions.add(new SpatialPainlessExtension()); // module: spatial | ||
extensions.add(new WildcardPainlessExtension()); // module: wildcard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these 4-extentions are missing ones.
When running integration tests, I get cannot resolve symbol [Processors]
(below log is with a bit more context) error which I believe belong to this painless ProcessorsWhitelistExtension
.
logstash-1 | Caused by: java.lang.IllegalArgumentException: cannot resolve symbol [Processors]
logstash-1 | at org.elasticsearch.painless.PainlessScript$Script.compile(long bytes = Processors.bytes(params['size']); ctx.size_in_bytes = bytes;:24) ~[?:?]
logstash-1 | at org.elasticsearch.painless.phase.DefaultSemanticAnalysisPhase.visitCall(DefaultSemanticAnalysisPhase.java:3233)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we will need to find a way to apply #162 on the bridge-side in Elasticsearch.
|
||
sourceAndMetadata.put(IngestDocument.Metadata.VERSION.getFieldName(), Objects.requireNonNullElse(sourceVersion, 1L)); | ||
// TODO: make IngestDocument.Metadata.VERSION.getFieldName() available | ||
sourceAndMetadata.put("_version", Objects.requireNonNullElse(sourceVersion, 1L)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wrap IngestDocument.Metadata
as well. For now I wanted fully isolate plugin from the IngestDocument
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of adding a public static final class Constants
or similar to the IngestDocumentBridge
:
// public class IngestDocumentBridge extends StableBridgeAPI.Proxy<IngestDocument> {
public static final class Constants {
private Constants() {}
public static final String METADATA_VERSION_FIELD_NAME = IngestDocument.Metadata.VERSION.getFieldName();
// ...
}
// }
return new GeoIpProcessor.Factory("geoip", this.ipDatabaseProvider) | ||
.create(processorFactories, tag, description, config, projectId); | ||
Map<String, Object> config) throws Exception { | ||
return ProcessorBridge.wrap(new GeoIpProcessor.Factory("geoip", this.ipDatabaseProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will move rg.elasticsearch.ingest.geoip.GeoIpProcessor
usage separately with GeoIP task.
src/main/java/co/elastic/logstash/filters/elasticintegration/ingest/PipelineProcessor.java
Outdated
Show resolved
Hide resolved
import org.elasticsearch.ingest.Processor; | ||
import org.elasticsearch.plugins.IngestPlugin; | ||
import org.elasticsearch.logstashbridge.ingest.ProcessorBridge; | ||
import org.elasticsearch.logstashbridge.plugins.IngestPluginBridge; | ||
import org.elasticsearch.license.XPackLicenseState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to move all processors (including this source piece) to ES with next iterations.
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
0707b1b
to
bc9af21
Compare
src/main/java/co/elastic/logstash/filters/elasticintegration/PipelineConfigurationFactory.java
Show resolved
Hide resolved
src/main/java/co/elastic/logstash/filters/elasticintegration/IngestDuplexMarshaller.java
Show resolved
Hide resolved
18333c6
to
5a96c8c
Compare
9820699
to
9bdad09
Compare
gradle.properties
Outdated
@@ -1,2 +1,3 @@ | |||
LOGSTASH_PATH=../../logstash | |||
ELASTICSEARCH_TREEISH=main | |||
ELASTICSEARCH_REPO=mashhurs/elasticsearch | |||
ELASTICSEARCH_TREEISH=move-to-bridge-stable-api-investigation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporary PoC, will be rolled back to origin once upstream PR is merged.
…s. Removes required upwrap interface since upstream moved to default method. Utilizes metadata version field since upstream made it accessible.
09c332c
to
9b0b557
Compare
fa3468c
to
53ded41
Compare
@@ -307,10 +307,6 @@ task shadeElasticsearchIngestGeoIpModule(type: com.github.jengelman.gradle.plugi | |||
|
|||
mergeServiceFiles() | |||
|
|||
String shadeNamespace = "org.elasticsearch.ingest.geoip.shaded" | |||
relocate('com.fasterxml.jackson', "${shadeNamespace}.com.fasterxml.jackson") | |||
relocate('com.maxmind', "${shadeNamespace}.com.maxmind") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get "No X class found" exceptions if we relocate them and I am not seeing org.elasticsearch.ingest.geoip.shaded.com.fasterxml.jackson.*
usages.
@@ -1,2 +1,3 @@ | |||
LOGSTASH_PATH=../../logstash | |||
ELASTICSEARCH_TREEISH=main | |||
ELASTICSEARCH_REPO=mashhurs/elasticsearch | |||
ELASTICSEARCH_TREEISH=logstash-bridge-geoip-interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: this will be removed once upstream PR is merged.
|
||
import org.elasticsearch.ingest.geoip.IpDatabase; | ||
|
||
public interface ValidatableIpDatabase extends IpDatabase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No direct usage.
|
||
import java.util.Map; | ||
|
||
public class RedactPlugin extends Plugin implements IngestPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the upstream bridge.
import org.elasticsearch.cluster.project.ProjectResolver; | ||
import org.elasticsearch.core.CheckedRunnable; | ||
|
||
public class PluginProjectResolver implements ProjectResolver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the bridge, ProjectIdResolverBridge
class.
…s to logstash-bridge.
💔 Build Failed
Failed CI StepsHistory
|
Description
This PR highlights the initial changes required to move to the Bridge Stable API. However, this doesn't mean these changes will be enough for the plugin to be a in normal working state. Rather, this changes highlight how does current bridge Spable API state align with plugin requirements. Based on the discussions on these changes, we may need to adjust the plan (sub-tasks).
Next steps
elastic_integration
plugin usage to ES logstash-bridge. elasticsearch#131486PLEASE IGNORE:
gradle.build
changes which are already counted in [Move to ES Bridge] Buildlogstash-bridge
and include in the plugin #330, this is a requirement to call Bridge Stable API interfaces.